-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More keychain fixes #1494
base: master
Are you sure you want to change the base?
More keychain fixes #1494
Conversation
Sources/Plasma/FeatureLib/pfPasswordStore/pfPasswordStore_Apple.cpp
Outdated
Show resolved
Hide resolved
Sources/Plasma/FeatureLib/pfPasswordStore/pfPasswordStore_Apple.cpp
Outdated
Show resolved
Hide resolved
Sources/Plasma/FeatureLib/pfPasswordStore/pfPasswordStore_Apple.cpp
Outdated
Show resolved
Hide resolved
Sources/Plasma/FeatureLib/pfPasswordStore/pfPasswordStore_Apple.cpp
Outdated
Show resolved
Hide resolved
dispatch_async(_keychainQueue, ^{ | ||
pfPasswordStore* store = pfPasswordStore::Instance(); | ||
if (self.rememberPassword) | ||
store->SetPassword(username, password); | ||
else | ||
store->SetPassword(username, ST::string()); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this is enqueuing the pfPasswordStore
function call to happen on a secondary thread. The goal of pfPasswordStore::Instance()
is to return a platform agnostic password store. The expectation is that all of the password stores have the same interface. The interface of the Win32 and Linux password stores is thread-agnostic. Therefore, it is important that the Apple password store be thread-agnostic as well. Thus, the worker thread fiddling needs to be done in pfPasswordStore_APPLE
so that the interfaces of all password stores match exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be complicated in the case of retrieving the password though (you could probably do something with std::promise
and changing the interface to return a std::future<ST::string>
but that's a bit complicated...) 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For extra fun - reading the password seems fine? It's the writing that is the issue. The dispatch threading could be moved to the password store class itself (or be replaced by C++ threading but that's a longer effort.)
FWIW - I don't know why the debugger throws this warning at runtime. They warn it could stall the main thread but don't say why. We trigger the keychain unlock dialog (if needed) at launch. It could be that if the user waits long enough, the keychain will lock, setting the password will need a dialog, and the main thread will be held.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking back in again - should re-arranging the underlying API to handle threading with promises be part of this PR? I can make the change - but I just want to make sure that's a direction we're all in agreement on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice if we could get away with not using a defferred (read std::promise
) for reading the password. IMO, it's ok if the main thread stalls there - I'm assuming that macOS has stolen the thread to prompt the user for some interaction. I would prefer to see the password saving dispatched inside pfPasswordStore
, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without knowing more about Apple's definition of blocking - I agree that this probably is fine on the main thread.
However - and this is what caused this PR - Xcode flags a runtime error/warning when we make this call off the main thread. So this issue is currently leading to us not getting clean runs out of Xcode.
f004031
to
f032344
Compare
I think this is stuck on the "should asyncronisity be baked in at an API level" question. |
f032344
to
a8bdbe5
Compare
I'm circling back to this PR as I was running into some of these issues when testing deploying a server the other day. I think where we got stuck is the main thread issues. Apple does not want us using the main thread - to the point where this gets raised as a runtime warning within Xcode. I'm not entirely clear on why not calling on the main thread is preferred. I can continue to investigate. As stated in the previous comment chain, I think we'd maybe be fine using the main thread to store passwords. But it would be nice to know why Apple thinks we shouldn't be safe. And it would be nice to not have the runtime warning. |
Minor fixes